Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add partial aarch64 support to libuser #566

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

leo60228
Copy link

This is able to make libuser compile with the included target JSON. It does not yet include a crt0 or syscall stub, though preparations for that have been made (I'm not experienced enough with aarch64 assembly).

aarch64 requires syscall numbers be immediates, so a standard function
can't be used here.
This was created by diffing the i386 JSON with i686-unknown-linux-gnu,
and applying relevant changes to aarch64-unknown-linux-gnu.
@leo60228
Copy link
Author

I've implemented a syscall stub, it was easier than I thought.

Copy link
Member

@roblabla roblabla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really good!

libuser/src/syscalls.rs Outdated Show resolved Hide resolved
libuser/src/syscalls.rs Outdated Show resolved Hide resolved
libuser/src/syscalls.rs Outdated Show resolved Hide resolved
libuser/src/syscalls.rs Outdated Show resolved Hide resolved
libuser/src/syscalls.rs Show resolved Hide resolved
libuser/src/threads.rs Show resolved Hide resolved
Copy link
Member

@Orycterope Orycterope left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Omg thanks so much

@@ -203,6 +204,18 @@ fn get_my_tls_region() -> *mut TLS {
tls
}

/// Get a pointer to this thread's [TLS] region pointed to by `fs`, translated to the flat-memory model.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update the comment

///
/// The routine to call and its argument are expected to be found in this `ThreadContext`.
#[cfg(not(target_arch = "x86"))]
extern fn thread_trampoline(thread_context_addr: usize) -> ! {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

almost certainly not valid for aarch64.

This deserves a huge

// todo: figure out aarch64 elf TLS

libuser/src/threads.rs Show resolved Hide resolved
libuser/src/threads.rs Show resolved Hide resolved
Comment on lines 73 to 79
eax: usize,
ebx: usize,
ecx: usize,
edx: usize,
esi: usize,
edi: usize,
ebp: usize,
Copy link
Member

@Orycterope Orycterope Mar 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@roblabla , I need your confirmation on this, but IIRC the sole reason we use a global asm for the syscall is because we need to pass ebp as our sixth argument, but trying to backup+set/restore it in an inline asm block was unfeasible, because Rust needs this register to exit the inline asm block and restore regular context (or something like that).

This struct is only used as an argument to the global asm syscall_iner. This is pretty inefficient, since to do a syscall you must:

  1. push all 6 arguments on the stack, and call syscall. (this is standard i386 ABI)
  2. syscall copies those arguments to fill a new Register structure, and calls syscall_inner
  3. syscall_inner consumes this struct, and puts every field in a register.
  4. int 0x80
  5. syscall_inner saves the return registers state to the Register structure, and returns.

On aarch64, where we have plenty of registers, this whole dance seems unnecessary, and I'd like to see the syscall function simply be a wrapper around a single inline asm instruction, that define its input/output registers properly (e.g. nr -> r0) and let llvm do all the copying for us.

In this context, the Register struct is very i386 specific, and doesn't need to be made generic.

I'd like to hear roblabla's opinion this, but if he agrees with me I'd recommend dropping this commit entirely

#[cfg(target_arch = "aarch64")]
asm!(
"svc $7"
: "={x0}"(registers.nr),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wow, you did exactly what I was discussing in the previous comment 😅

For aarch64, I don't see the need for copying arguments to the Registers struct. Can't you just directly use nr ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought Registers was in the public API? If it isn't, that's unnecessary.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was, but I think that's a mistake, it should be defined only for i386

@todo
Copy link

todo bot commented Mar 20, 2020

figure out aarch64 TLS

// TODO: figure out aarch64 TLS
debug!("starting from new thread, context at address {:#010x}", thread_context_addr);
// first save the address of our context in our TLS region
unsafe {
// safe: - get_my_tls returns a valid 0x200 aligned ptr,
// - .ptr_thread_context is correctly aligned in the TLS region to usize,
// - we're a private fn, thread_context_addr is guaranteed by our caller to point to the context.
(*get_my_tls_region()).ptr_thread_context = thread_context_addr
};
// use get_my_thread_context to create a ref for us


This comment was generated by todo based on a TODO comment in 708cb35 in #566. cc @leo60228.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants